-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
theme: Adds very basic theme support. #112
base: master
Are you sure you want to change the base?
Conversation
bb58e9f
to
b9803f8
Compare
e21adb1
to
a504757
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good, rudimentary 1080i support may be a decent idea but doesn't have to be within the scope of this PR
|
||
std::ifstream themeFile(themeFilePath); | ||
nlohmann::json json; | ||
// FIXME: Once nxdk supports C++ Exceptions, this needs to be put in a try-catch block! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is supported now is it not?
public: | ||
struct MenuTheme { | ||
std::string font; | ||
// TODO: Actually support this in Font. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: font not Font
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure I was referring to the Font
class, so I think the capitalization is correct. Making it "font" would be ambiguous with the member var just above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also sounds good
{ "mount", nlohmann::json(o.mount) }, | ||
#ifdef NXDK | ||
{ "network", nlohmann::json(o.net) }, | ||
#endif | ||
{ "logging", nlohmann::json(o.logging) }, | ||
{ "homescreenConfig", nlohmann::json(o.homescreen) } }; | ||
{ "homescreen", nlohmann::json(o.homescreen) } }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: specific reason why this was changed? Listing it as config may make it easier for outside devs to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been a long time, but looking a few lines down at line 246 it's specifically looking for "homescreen" when reading the config file, so I think saving the config would always cause the default values to be used.
It also strikes me as more consistent to use "homescreen" in the same way that "logging", etc... is used rather than "loggingConfig".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: Sounds Good, voice to text is really losing today
Puts in the foundation for theme selection.
Resources/NeXThemes
and drops thenew_all
workaround by addingRESOURCES
toTARGET
.Theme
JSON parsing class.default
theme subdir and adds a .json definition file for them.Some nice followups would be:
Font
so the font color can be changed.HalQuickRebootRoutine
to return to dashboard XboxDev/nxdk#507 will need to be fixed)